Skip to content

Api unit tests#266

Merged
dbutenhof merged 7 commits intocloud-bulldozer:mainfrom
khandrew-redhat:api-unit-tests
Sep 17, 2025
Merged

Api unit tests#266
dbutenhof merged 7 commits intocloud-bulldozer:mainfrom
khandrew-redhat:api-unit-tests

Conversation

@khandrew-redhat
Copy link
Copy Markdown
Collaborator

@khandrew-redhat khandrew-redhat commented Aug 18, 2025 •
edited
Loading

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

Created unit test files...

  • test_api.py
  • test_hasher.py
  • test_hce.py
  • test_ocm.py
  • test_ocp.py
  • test_ols.py
  • test_quay.py
  • test_telco.py
  • test_utils.py
    ...all for backend. Achieved 100% test coverage on all (with the exception of utils.py at 99%).

Assisted-by: Cursor

Related Tickets & Documents

  • Related Issue # PANDA 944
  • Related Issue # PANDA 945

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please describe the System Under Test.
  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

@dbutenhof
Copy link
Copy Markdown
Collaborator

dbutenhof commented Aug 18, 2025

Here's something I should have thought to suggest before any PR:

$ cd backend
$ black .
$ isort .
$ flake8 .

(Ideally inside your virtual environment to be sure you have the black, isort, and flake8 tools available with the right versions.) You can set up git "pre-commit hooks" locally to run tools for you; although I've always preferred to do it manually. (You just have to get into the habit, and I do occasionally forget. 😁)

The test_api.py file is showing formatting errors. (Cursor isn't really good about lint checking and correction: I've noticed that it often tries, repeatedly, and gives up because it's never content with its attempts. I tend to just fix the formatting myself manually, and I haven't really looked into whether I can fix the configuration to do it right... 😦)

I'm also seeing unit test failures -- I'm worried that your local environment setup is somehow different from the GitHub CI environment, which is unfortunate.

@khandrew-redhat khandrew-redhat marked this pull request as draft August 18, 2025 14:56
@khandrew-redhat khandrew-redhat marked this pull request as ready for review September 2, 2025 07:59
dbutenhof
dbutenhof requested changes Sep 3, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about the mingling of the mock Elasticsearch and ElasticService ... I think this needs to be cleaned up to avoid future problems. There are a few other comments, but mostly this looks good aside from that main issue.

I think that a new split-off FakeElasticService might be essentially just the new methods you added to FakeAsyncElasticsearch, so it may not be that hard to fix.

(And sorry I didn't finish this yesterday ... I got halfway through before sprint planning, then got distracted and didn't get back to it!)

dbutenhof
dbutenhof reviewed Sep 3, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments

dbutenhof
dbutenhof reviewed Sep 3, 2025
View reviewed changes
dbutenhof
dbutenhof reviewed Sep 4, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few possible "refinements" in the open discussions, so let's resolve those before we finalize ... but this is looking good!

dbutenhof
dbutenhof approved these changes Sep 17, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

FYI -- for future reference, let's adopt a convention that's been successful in the past ... if only the reviewer marks comments as "resolved" it's easier for that reviewer to check them off against subsequent changes since GitHub makes them easily accessible through the "Conversations" pulldown ... whereas finding them in the Conversation tab can be a lot more awkward.

@dbutenhof dbutenhof merged commit 471fea8 into cloud-bulldozer:main Sep 17, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbutenhof dbutenhof dbutenhof approved these changes

Assignees

@khandrew-redhat khandrew-redhat

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@khandrew-redhat @dbutenhof